📖 Add a development.md#1147
📖 Add a development.md#1147openshift-merge-bot[bot] merged 2 commits intoopen-cluster-management-io:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded@qiujian16 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a new development guide at Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1147 +/- ##
=======================================
Coverage 58.20% 58.20%
=======================================
Files 211 211
Lines 20791 20791
=======================================
Hits 12101 12101
Misses 7623 7623
Partials 1067 1067
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
development.md (2)
72-72: Add language to fenced code block (markdownlint MD040).Apply this diff:
-``` +```text
79-80: Use proper “gRPC” capitalization.Apply this diff:
-│ ├── server/ # GRPC server +│ ├── server/ # gRPC server
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(1 hunks)development.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
development.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...# Table of Contents - Project Overview - [Development Environment Setup](#developm...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...erview) - Development Environment Setup - Code Organization -...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...-environment-setup) - Code Organization - [Development Workflow](#development-workf...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...de-organization) - Development Workflow - [Code Standards and Style](#code-standard...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...nt-workflow) - Code Standards and Style - Testing Guidelines...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...andards-and-style) - Testing Guidelines - API Development - [Bu...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...(#testing-guidelines) - API Development - Build and Release -...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...](#api-development) - Build and Release - Troubleshooting ## P...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ...y Technologies - Language: Go 1.24+ - Framework: Kubernetes operators with c...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...rnetes operators with controller-runtime - Build System: Make with OpenShift buil...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...m**: Make with OpenShift build machinery - Testing: Ginkgo/Gomega for BDD-style t...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...ing**: Ginkgo/Gomega for BDD-style tests - Packaging: Helm charts and OLM bundles...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...tion guide](https://go.dev/doc/install)) - Docker or Podman (container engine) - [K...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...)) - Docker or Podman (container engine) - Kind (local ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...igs.k8s.io/) (local Kubernetes clusters) - [kubectl](https://kubernetes.io/docs/task...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...s.io/docs/tasks/tools/) (Kubernetes CLI) - [clusteradm](https://github.com/open-clus...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...g/common/: Shared utilities and helpers - pkg/registration/hub/: Hub-side registration controllers - p...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...hub/: Hub-side registration controllers - pkg/registration/spoke/: Managed cluster registration logic - ...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...ke/: Managed cluster registration logic - pkg/work/hub/: Hub-side work distribution - pkg/work...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...g/work/hub/: Hub-side work distribution - pkg/work/spoke/: Managed cluster work application - pk...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...poke/: Managed cluster work application - pkg/placement/controllers/`: Placement scheduling and decision logi...
(QB_NEW_EN)
[grammar] ~103-~103: Ensure spelling is correct
Context: ...ision logic - pkg/addon/controllers/: Addon lifecycle management controllers ## De...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~155-~155: There might be a mistake here.
Context: ...nventions**: Use gofmt, go vet, and golangci-lint - Variable naming: Use camelCase for loc...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...ust be organized in the following order: 1. Standard library packages 2. Third-party...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...wing order: 1. Standard library packages 2. Third-party packages 3. OCM packages ...
(QB_NEW_EN)
[grammar] ~165-~165: There might be a mistake here.
Context: ...library packages 2. Third-party packages 3. OCM packages ```go import ( // Stan...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...n make test-e2e ``` E2E test variables: - IMAGE_TAG: Tag for container images (default: lat...
(QB_NEW_EN)
[grammar] ~251-~251: There might be a mistake here.
Context: ...g for container images (default: latest) - KLUSTERLET_DEPLOY_MODE: Deployment mode (Default, Singleton, S...
(QB_NEW_EN)
[grammar] ~252-~252: There might be a mistake here.
Context: ...de (Default, Singleton, SingletonHosted) - MANAGED_CLUSTER_NAME: Name of the managed cluster (default: ...
(QB_NEW_EN)
[grammar] ~253-~253: There might be a mistake here.
Context: ... the managed cluster (default: cluster1) - KUBECONFIG: Must be set to point to test cluster ...
(QB_NEW_EN)
[grammar] ~258-~258: There might be a mistake here.
Context: ... *_test.go files alongside source code - Integration tests: test/integration/...
(QB_NEW_EN)
[grammar] ~259-~259: There might be a mistake here.
Context: ...n tests**: test/integration/ directory - E2E tests: test/e2e/ directory - **T...
(QB_NEW_EN)
[grammar] ~260-~260: There might be a mistake here.
Context: ...y - E2E tests: test/e2e/ directory - Test utilities: test/framework/ and ...
(QB_NEW_EN)
[grammar] ~266-~266: There might be a mistake here.
Context: ...e test names using Describe, Context, and It - Use table-driven tests for multi...
(QB_NEW_EN)
[grammar] ~297-~297: There might be a mistake here.
Context: ... 4. Remove replace directive before submitting PR ### Update Process The `make updat...
(QB_NEW_EN)
[grammar] ~301-~301: There might be a mistake here.
Context: ...cess The make update command handles: - CRD copying from api repository - Helm c...
(QB_NEW_EN)
[grammar] ~390-~390: There might be a mistake here.
Context: ...unctionality 4. Run quality checks: make verify test-unit 5. Submit pull request with DCO sign-off ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
development.md
72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: verify
- GitHub Check: integration
- GitHub Check: build
- GitHub Check: unit
🔇 Additional comments (3)
development.md (2)
188-200: All Makefile targets in development.md are validI’ve confirmed that every target listed under lines 188–200 of
development.mdexists in the Makefile:
verify– defined at Makefile line 108verify-gocilint– defined at Makefile line 88verify-fmt-imports– defined at Makefile line 98verify-crds– defined at Makefile line 85fmt-imports– defined at Makefile line 95No documented targets are missing or renamed.
70-104: All referenced directories and links validatedAll advertised directories (
solutions/,cmd/,pkg/,manifests/,deploy/,test/,test/e2e,test/integration,cmd/addon,cmd/placement,cmd/registration,cmd/registration-operator,cmd/server) and theCONTRIBUTING.mdfile exist. Thesolutions/setup-dev-environmentfolder is present, and the README link to it is correct. No dead paths remain.README.md (1)
105-106: LGTM — good discoverability improvement.
development.md
Outdated
| - **Error handling**: Always handle errors explicitly, use `errors.Wrap()` for context | ||
| - **Comments**: Write clear comments for exported functions and complex logic |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer stdlib error wrapping over pkg/errors.
Recommend fmt.Errorf with %w (and errors.Join) instead of errors.Wrap().
Apply this diff:
-- **Error handling**: Always handle errors explicitly, use `errors.Wrap()` for context
+- **Error handling**: Always handle errors explicitly; wrap with `fmt.Errorf("context: %w", err)` and use `errors.Join(...)` when aggregating errors📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - **Error handling**: Always handle errors explicitly, use `errors.Wrap()` for context | |
| - **Comments**: Write clear comments for exported functions and complex logic | |
| - **Error handling**: Always handle errors explicitly; wrap with `fmt.Errorf("context: %w", err)` and use `errors.Join(...)` when aggregating errors | |
| - **Comments**: Write clear comments for exported functions and complex logic |
🤖 Prompt for AI Agents
In development.md around lines 157 to 158, the guidance currently recommends
using errors.Wrap() from pkg/errors; update the text to prefer the Go standard
library error wrapping: use fmt.Errorf with %w for single wrapped errors and
errors.Join for combining multiple errors, and remove or deprecate mention of
errors.Wrap; also add a short note to update import guidance (use "fmt" and
"errors" from the stdlib) and a brief example suggestion showing
fmt.Errorf("context: %w", err) and errors.Join(err1, err2) for clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
development.md (1)
39-44: Unify Go version (pinned vs. plus).Key Technologies says Go 1.24.0, but Prerequisites says 1.24+. Align to the pinned version.
- - Go 1.24+ ([installation guide](https://go.dev/doc/install)) + - Go 1.24.0 ([installation guide](https://go.dev/doc/install))
🧹 Nitpick comments (2)
development.md (2)
72-93: Add language hint to fenced block (markdownlint MD040).Label the directory tree as text to satisfy markdownlint and improve rendering.
-``` +```text ... -``` +```
300-305: Add an explicit “do not edit CRDs here” warning.Per project practice, CRDs are generated from the api repo. Add a guard note to prevent accidental edits. (Using retrieved learnings from prior PRs.)
The `make update` command handles: - CRD copying from api repository - Helm chart updates - Operator manifest generation - ClusterServiceVersion updates + +> Note: Do not edit CRDs under deploy/*/crds directly. Make schema changes in the +> open-cluster-management-io/api repository, regenerate there, then run `make update` +> in this repo to sync generated artifacts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
development.md(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
PR: open-cluster-management-io/ocm#1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
development.md
🪛 LanguageTool
development.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...# Table of Contents - Project Overview - [Development Environment Setup](#developm...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...erview) - Development Environment Setup - Code Organization -...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...-environment-setup) - Code Organization - [Development Workflow](#development-workf...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...de-organization) - Development Workflow - [Code Standards and Style](#code-standard...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...nt-workflow) - Code Standards and Style - Testing Guidelines...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...andards-and-style) - Testing Guidelines - API Development - [Bu...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...(#testing-guidelines) - API Development - Build and Release -...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...](#api-development) - Build and Release - Troubleshooting ## P...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ... Technologies - Language: Go 1.24.0 - Framework: Kubernetes operators with c...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...rnetes operators with controller-runtime - Build System: Make with OpenShift buil...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...m**: Make with OpenShift build machinery - Testing: Ginkgo/Gomega for BDD-style t...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...ing**: Ginkgo/Gomega for BDD-style tests - Packaging: Helm charts and OLM bundles...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...tion guide](https://go.dev/doc/install)) - Docker or Podman (container engine) - [K...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...)) - Docker or Podman (container engine) - Kind (local ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...igs.k8s.io/) (local Kubernetes clusters) - [kubectl](https://kubernetes.io/docs/task...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...s.io/docs/tasks/tools/) (Kubernetes CLI) - [clusteradm](https://github.com/open-clus...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...g/common/: Shared utilities and helpers - pkg/registration/hub/: Hub-side registration controllers - p...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...hub/: Hub-side registration controllers - pkg/registration/spoke/: Managed cluster registration logic - ...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...ke/: Managed cluster registration logic - pkg/work/hub/: Hub-side work distribution - pkg/work...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...g/work/hub/: Hub-side work distribution - pkg/work/spoke/: Managed cluster work application - pk...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...poke/: Managed cluster work application - pkg/placement/controllers/`: Placement scheduling and decision logi...
(QB_NEW_EN)
[grammar] ~103-~103: Ensure spelling is correct
Context: ...ision logic - pkg/addon/controllers/: Addon lifecycle management controllers ## De...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~155-~155: There might be a mistake here.
Context: ...nventions**: Use gofmt, go vet, and golangci-lint - Variable naming: Use camelCase for loc...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ...ust be organized in the following order: 1. Standard library packages 2. Third-party...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...wing order: 1. Standard library packages 2. Third-party packages 3. OCM packages ...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...library packages 2. Third-party packages 3. OCM packages ```go import ( // Stan...
(QB_NEW_EN)
[grammar] ~249-~249: There might be a mistake here.
Context: ...n make test-e2e ``` E2E test variables: - IMAGE_TAG: Tag for container images (default: lat...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...g for container images (default: latest) - KLUSTERLET_DEPLOY_MODE: Deployment mode (Default, Singleton, S...
(QB_NEW_EN)
[grammar] ~251-~251: There might be a mistake here.
Context: ...de (Default, Singleton, SingletonHosted) - MANAGED_CLUSTER_NAME: Name of the managed cluster (default: ...
(QB_NEW_EN)
[grammar] ~252-~252: There might be a mistake here.
Context: ... the managed cluster (default: cluster1) - KUBECONFIG: Must be set to point to test cluster ...
(QB_NEW_EN)
[grammar] ~257-~257: There might be a mistake here.
Context: ... *_test.go files alongside source code - Integration tests: test/integration/...
(QB_NEW_EN)
[grammar] ~258-~258: There might be a mistake here.
Context: ...n tests**: test/integration/ directory - E2E tests: test/e2e/ directory - **T...
(QB_NEW_EN)
[grammar] ~259-~259: There might be a mistake here.
Context: ...y - E2E tests: test/e2e/ directory - Test utilities: test/framework/ and ...
(QB_NEW_EN)
[grammar] ~265-~265: There might be a mistake here.
Context: ...e test names using Describe, Context, and It - Use table-driven tests for multi...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ... 4. Remove replace directive before submitting PR ### Update Process The `make updat...
(QB_NEW_EN)
[grammar] ~300-~300: There might be a mistake here.
Context: ...cess The make update command handles: - CRD copying from api repository - Helm c...
(QB_NEW_EN)
[grammar] ~389-~389: There might be a mistake here.
Context: ...unctionality 4. Run quality checks: make verify test-unit 5. Submit pull request with DCO sign-off ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
development.md
72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: unit
- GitHub Check: build
- GitHub Check: integration
- GitHub Check: verify
🔇 Additional comments (3)
development.md (3)
241-247: Nice: updated kind kubeconfig flow matches current kind behavior.The deprecated get kubeconfig-path is replaced correctly with writing kubeconfig to a file and exporting KUBECONFIG.
70-93: No changes needed: code organization tree accurately reflects repository directories.
All listed paths (cmd/*,pkg/*,manifests,deploy,test/*,solutions) exist in the repo.
189-199: Makefile target names in development.md are correct; no updates needed
Signed-off-by: Jian Qiu <jqiu@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
development.md (2)
54-56: Pin clusteradm to a release tag; avoid “main” and “latest”Use a concrete tag for reproducibility and replace bundle-version “latest” with an explicit version.
- curl -fsSL https://raw.githubusercontent.com/open-cluster-management-io/clusteradm/main/hack/install.sh | bash - clusteradm init --wait --bundle-version="latest" + # Replace <TAG> with a concrete release (e.g., vX.Y.Z) + curl -fsSL https://raw.githubusercontent.com/open-cluster-management-io/clusteradm/<TAG>/hack/install.sh | bash + clusteradm init --wait --bundle-version="<BUNDLE_VERSION>"- clusteradm join --hub-token <token> --hub-apiserver <url> --wait --cluster-name cluster1 --force-internal-endpoint-lookup --bundle-version="latest" + clusteradm join --hub-token <token> --hub-apiserver <url> --wait --cluster-name cluster1 --force-internal-endpoint-lookup --bundle-version="<BUNDLE_VERSION>"Also applies to: 60-62
29-39: Unify Go version: pin to one exact valueYou list Go 1.24.0 (Line 29) but “Go 1.24+” (Line 39). Please make them consistent and pinned to the exact version to match go.mod/CI.
Apply:
- - Go 1.24+ ([installation guide](https://go.dev/doc/install)) + - Go 1.24.0 ([installation guide](https://go.dev/doc/install))
🧹 Nitpick comments (4)
development.md (4)
168-183: Avoid import name collision with stdlib errorsImporting k8s API errors unaliased conflicts with the stdlib “errors” in examples. Alias to apierrors to set good precedent.
- "k8s.io/apimachinery/pkg/api/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors"
72-93: Add language to fenced code block (markdownlint MD040)Specify a language for the directory tree to satisfy linters.
-``` +```text ...--- `241-247`: **Add kind cluster creation step for E2E section** You export kubeconfig for “ocm-e2e” without showing its creation. Add the create step to avoid confusion. ```diff ```bash -# 1. Create kind cluster and set KUBECONFIG -kind get kubeconfig --name ocm-e2e > /tmp/kubeconfig-ocm-e2e +# 1. Create kind cluster and set KUBECONFIG +kind create cluster --name ocm-e2e +kind get kubeconfig --name ocm-e2e > /tmp/kubeconfig-ocm-e2e export KUBECONFIG=/tmp/kubeconfig-ocm-e2e
300-305: Call out: don’t edit CRD YAMLs here; update API repo insteadReinforce that CRDs are generated from open-cluster-management.io/api, aligning with project practice.
The `make update` command handles: - CRD copying from api repository - Helm chart updates - Operator manifest generation - ClusterServiceVersion updates +> Note: Do not edit CRD YAMLs in this repo directly. Make schema changes in +> the open-cluster-management.io/api repository, run its generators, then +> update this repo via `make update`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
README.md(1 hunks)development.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-28T01:58:50.021Z
Learnt from: skeeey
PR: open-cluster-management-io/ocm#1146
File: deploy/cluster-manager/chart/cluster-manager/crds/0000_01_operator.open-cluster-management.io_clustermanagers.crd.yaml:168-175
Timestamp: 2025-08-28T01:58:50.021Z
Learning: In the OCM project, CRD schemas come from the open-cluster-management.io/api dependency. Changes to CRD schemas should be made upstream in that API package, not in the local CRD YAML files which are generated/copied from the API types.
Applied to files:
development.md
🪛 LanguageTool
development.md
[grammar] ~7-~7: There might be a mistake here.
Context: ...# Table of Contents - Project Overview - [Development Environment Setup](#developm...
(QB_NEW_EN)
[grammar] ~8-~8: There might be a mistake here.
Context: ...erview) - Development Environment Setup - Code Organization -...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...-environment-setup) - Code Organization - [Development Workflow](#development-workf...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...de-organization) - Development Workflow - [Code Standards and Style](#code-standard...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...nt-workflow) - Code Standards and Style - Testing Guidelines...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...andards-and-style) - Testing Guidelines - API Development - [Bu...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...(#testing-guidelines) - API Development - Build and Release -...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...](#api-development) - Build and Release - Troubleshooting ## P...
(QB_NEW_EN)
[grammar] ~29-~29: There might be a mistake here.
Context: ... Technologies - Language: Go 1.24.0 - Framework: Kubernetes operators with c...
(QB_NEW_EN)
[grammar] ~30-~30: There might be a mistake here.
Context: ...rnetes operators with controller-runtime - Build System: Make with OpenShift buil...
(QB_NEW_EN)
[grammar] ~31-~31: There might be a mistake here.
Context: ...m**: Make with OpenShift build machinery - Testing: Ginkgo/Gomega for BDD-style t...
(QB_NEW_EN)
[grammar] ~32-~32: There might be a mistake here.
Context: ...ing**: Ginkgo/Gomega for BDD-style tests - Packaging: Helm charts and OLM bundles...
(QB_NEW_EN)
[grammar] ~39-~39: There might be a mistake here.
Context: ...tion guide](https://go.dev/doc/install)) - Docker or Podman (container engine) - [K...
(QB_NEW_EN)
[grammar] ~40-~40: There might be a mistake here.
Context: ...)) - Docker or Podman (container engine) - Kind (local ...
(QB_NEW_EN)
[grammar] ~41-~41: There might be a mistake here.
Context: ...igs.k8s.io/) (local Kubernetes clusters) - [kubectl](https://kubernetes.io/docs/task...
(QB_NEW_EN)
[grammar] ~42-~42: There might be a mistake here.
Context: ...s.io/docs/tasks/tools/) (Kubernetes CLI) - [clusteradm](https://github.com/open-clus...
(QB_NEW_EN)
[grammar] ~97-~97: There might be a mistake here.
Context: ...g/common/: Shared utilities and helpers - pkg/registration/hub/: Hub-side registration controllers - p...
(QB_NEW_EN)
[grammar] ~98-~98: There might be a mistake here.
Context: ...hub/: Hub-side registration controllers - pkg/registration/spoke/: Managed cluster registration logic - ...
(QB_NEW_EN)
[grammar] ~99-~99: There might be a mistake here.
Context: ...ke/: Managed cluster registration logic - pkg/work/hub/: Hub-side work distribution - pkg/work...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...g/work/hub/: Hub-side work distribution - pkg/work/spoke/: Managed cluster work application - pk...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ...poke/: Managed cluster work application - pkg/placement/controllers/`: Placement scheduling and decision logi...
(QB_NEW_EN)
[grammar] ~103-~103: Ensure spelling is correct
Context: ...ision logic - pkg/addon/controllers/: Addon lifecycle management controllers ## De...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~155-~155: There might be a mistake here.
Context: ...nventions**: Use gofmt, go vet, and golangci-lint - Variable naming: Use camelCase for loc...
(QB_NEW_EN)
[grammar] ~162-~162: There might be a mistake here.
Context: ...ust be organized in the following order: 1. Standard library packages 2. Third-party...
(QB_NEW_EN)
[grammar] ~163-~163: There might be a mistake here.
Context: ...wing order: 1. Standard library packages 2. Third-party packages 3. OCM packages ...
(QB_NEW_EN)
[grammar] ~164-~164: There might be a mistake here.
Context: ...library packages 2. Third-party packages 3. OCM packages ```go import ( // Stan...
(QB_NEW_EN)
[grammar] ~249-~249: There might be a mistake here.
Context: ...n make test-e2e ``` E2E test variables: - IMAGE_TAG: Tag for container images (default: lat...
(QB_NEW_EN)
[grammar] ~250-~250: There might be a mistake here.
Context: ...g for container images (default: latest) - KLUSTERLET_DEPLOY_MODE: Deployment mode (Default, Singleton, S...
(QB_NEW_EN)
[grammar] ~251-~251: There might be a mistake here.
Context: ...de (Default, Singleton, SingletonHosted) - MANAGED_CLUSTER_NAME: Name of the managed cluster (default: ...
(QB_NEW_EN)
[grammar] ~252-~252: There might be a mistake here.
Context: ... the managed cluster (default: cluster1) - KUBECONFIG: Must be set to point to test cluster ...
(QB_NEW_EN)
[grammar] ~257-~257: There might be a mistake here.
Context: ... *_test.go files alongside source code - Integration tests: test/integration/...
(QB_NEW_EN)
[grammar] ~258-~258: There might be a mistake here.
Context: ...n tests**: test/integration/ directory - E2E tests: test/e2e/ directory - **T...
(QB_NEW_EN)
[grammar] ~259-~259: There might be a mistake here.
Context: ...y - E2E tests: test/e2e/ directory - Test utilities: test/framework/ and ...
(QB_NEW_EN)
[grammar] ~265-~265: There might be a mistake here.
Context: ...e test names using Describe, Context, and It - Use table-driven tests for multi...
(QB_NEW_EN)
[grammar] ~296-~296: There might be a mistake here.
Context: ... 4. Remove replace directive before submitting PR ### Update Process The `make updat...
(QB_NEW_EN)
[grammar] ~300-~300: There might be a mistake here.
Context: ...cess The make update command handles: - CRD copying from api repository - Helm c...
(QB_NEW_EN)
[grammar] ~389-~389: There might be a mistake here.
Context: ...unctionality 4. Run quality checks: make verify test-unit 5. Submit pull request with DCO sign-off ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
development.md
72-72: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: verify
- GitHub Check: build
🔇 Additional comments (1)
development.md (1)
189-198: All referenced Make targets exist
Confirmed that the Makefile definesverify-crds,verify-gocilint,verify-fmt-imports, andfmt-importstargets.
Signed-off-by: Jian Qiu <jqiu@redhat.com>
|
LGTM |
|
/lgtm |
openshift-merge-bot
bot
merged commit e4530ee
into
open-cluster-management-io:main
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit